-
Notifications
You must be signed in to change notification settings - Fork 4
Conversation
4511779
to
d5209e3
Compare
d5209e3
to
850e0d5
Compare
|
CakePHP 4 → 5 Upgrade Developer Guideline Scope
References
Repository-specific hotspots (found using IDE scan)
Checklist and code patterns to change
$query = $table->find()->order(['EnrollmentFlowSteps.ordr' => 'ASC']);
$query = $table->find()->orderBy(['EnrollmentFlowSteps.ordr' => 'ASC']);
$entity = $Table->get($id, ['contain' => $contain, 'fields' => $fields]);
$entity = $Table->get($id, contain: $contain, fields: $fields);
public function beforeSave(EventInterface $event, EntityInterface $entity, ArrayObject $options)
{
if ($this->needsSkip($entity)) {
return false; // or return true / return $response
}
}
public function beforeSave(EventInterface $event, EntityInterface $entity, ArrayObject $options)
{
if ($this->needsSkip($entity)) {
$event->setResult(false); // or set desired value
$event->stopPropagation(); // optional: only if you intend to stop other listeners
return; // explicitly return void
}
}
$rs = $this->paginate($query);
while ($rs->valid()) {
$o = $rs->current();
$rs->next();
}
$rs = $this->paginate($query);
while ($rs->items()->valid()) {
$o = $rs->items()->current();
$rs->items()->next();
}
class CousController extends StandardController {
// somewhere: $this->SomeTable = $this->fetchTable('Some');
}
#[\AllowDynamicProperties]
class CousController extends StandardController {
// or declare public properties explicitly
}
Search-and-Replace plan (do not apply to vendor/):
Examples from this repo (before → after)
->order(['EnrollmentFlowSteps.ordr' => 'ASC'])
→ ->orderBy(['EnrollmentFlowSteps.ordr' => 'ASC'])
->order(['EnrollmentFlowSteps.ordr'])
→ ->orderBy(['EnrollmentFlowSteps.ordr'])
->order(['Names.family', 'Names.given', 'Names.middle'])
→ ->orderBy(['Names.family', 'Names.given', 'Names.middle'])
->order(['name' => 'ASC'], true)
→ ->orderBy(['name' => 'ASC'], true)
$linkObj = $linkTable->get($link->value, ['contain' => $contain]);
→ $linkObj = $linkTable->get($link->value, contain: $contain);
while($rs->valid()) { $o = $rs->current(); /* … */ }
→ while($rs->items()->valid()) { $o = $rs->items()->current(); /* … */ }
if ($controller->calculatePermission()) {
// return true; (deprecated)
$event->setResult(true);
return;
}
#[\AllowDynamicProperties]
class CousController extends StandardController { } |
|
Bumped to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see anything after the vendor directory because github won't show me more than 3000 files, but that should just be the webroot directory (which I probably don't have much to say about anyway). If there's something in there that I should look at let me know via another channel.
| @@ -0,0 +1,11 @@ | |||
| # LdapConnector plugin for CakePHP | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LdapConnector is not currently a thing so this plugin should probably not be in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| @@ -44,6 +44,7 @@ | |||
| use Cake\ORM\TableRegistry; | |||
| use Cake\Utility\Hash; | |||
| use InvalidArgumentException; | |||
| use josegonzalez\Dotenv\Filter\NullFilter; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
| protected $RegistryAuth = null; | ||
| protected $Breadcrumb = null; | ||
|
|
||
| protected $Flash = null; | ||
| protected $FormProtection = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be reinventing a syntax Cake is moving away from. Is this just so we don't have to fix a bunch of references?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found the issue and fixed it. The issue was
- if(isset($this->RegistryAuth)) {
+ if ($this->components()->has('RegistryAuth')) {| @@ -33,8 +33,9 @@ | |||
| use Cake\Log\Log; | |||
| //use \App\Lib\Enum\PermissionEnum; | |||
|
|
|||
| #[\AllowDynamicProperties] | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this because of stuff we do, stuff Cake does, or both?
Given that dynamic properties are deprecated, do we need a (separate) plan for getting rid of them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably what we do. I am not sure if we can easily remove the decorator. I need to do more testing. Currently i am using it in 5 files.
| use Cake\Datasource\EntityInterface; | ||
| use Cake\Event\Event; | ||
| use Cake\Event\EventListenerInterface; | ||
| use Cake\ORM\RulesChecker; | ||
| use Cake\ORM\TableRegistry; | ||
| use Cake\Utility\Inflector; | ||
| use PHPUnit\Event\RuntimeException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we using PHPUnit\Event\RuntimeException and not just \RuntimeException?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
False import.
| @@ -29,16 +29,20 @@ | |||
|
|
|||
| namespace App\Lib\Events; | |||
|
|
|||
| use App\Lib\Traits\LabeledLogTrait; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that we have to declare the trait inside the class, what's the benefit of having a second declaration here that specifies the path? ie: Why are two lines better than one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
| // The documentation is ambiguous as to whether or not we need to return $rules. | ||
| // The API docs say yes but the example doesn't have it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment no longer seems relevant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
| return; | ||
| } else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting rid of this else clause makes it less clear that there are exactly two non-overlapping cases being handled here. Is there something else this change is supposed to be doing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no. reverting
| @@ -186,6 +186,7 @@ public function validationDefault(Validator $validator): Validator { | |||
| // Note that adopting an EIS Record briefly creates a second EIS Record for the same | |||
| // source key, so we shouldn't try to enforce uniqueness of the source key here. | |||
| // (See ExternalIdentitiesTable::adopt.) | |||
| // XXX Resyncing breaks things for OrcidSource | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this supposed to be committed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no. removed
Important changes:
example:
example:
$event->setResult()instead or$event->stopPropogation()to just stop the event propagation.example:
example:
example:
example